Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Components: Modal, add story #18083

Merged
merged 7 commits into from
Oct 31, 2019
Merged

Components: Modal, add story #18083

merged 7 commits into from
Oct 31, 2019

Conversation

ItsJonQ
Copy link

@ItsJonQ ItsJonQ commented Oct 23, 2019

Components: Modal, Add story

Screen Capture on 2019-10-23 at 13-37-27

This update adds a story for the Modal component.
The story showcases an updated and enhanced example from the Modal
README. It also includes a series of knobs to better demonstrate the
Modal props.

Screen Shot 2019-10-23 at 1 37 37 PM

Components: Modal, Add story
76a6848 
This update adds a story for the Modal component.
The story showcases an updated and enhanced example from the Modal
README. It also includes a series of knobs to better demonstrate the
Modal props.
@ItsJonQ ItsJonQ mentioned this pull request Oct 23, 2019
Closed
8 tasks
@ItsJonQ ItsJonQ changed the title Components: Modal, Add story Components: Modal, add story Oct 23, 2019
@gziolo gziolo added Storybook Storybook and its stories for components [Feature] UI Components Impacts or related to the UI component system labels Oct 28, 2019
@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Oct 29, 2019
@ItsJonQ
Copy link
Author

ItsJonQ commented Oct 29, 2019

@gziolo Updates pushed! I also updated the README.md for Modal to use useState instead of the previous pre-hook example.

gziolo
gziolo approved these changes Oct 30, 2019
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in my comment, this modal opens by default:
Screen Shot 2019-10-30 at 15 47 25

I see one accessibility-related issue. It might be a bug in the current implementation. We should double-check and file an issue if my guess is true.

In general, this work is great. My comments are nits 😍

packages/components/src/modal/stories/index.js Outdated Show resolved Hide resolved
packages/components/src/modal/stories/index.js Outdated Show resolved Hide resolved
@ItsJonQ
Copy link
Author

ItsJonQ commented Oct 30, 2019

@gziolo Refactored the Modal story so that it doesn't open by default. It looks like that resolves the a11y issue as well 💪

@gziolo
Copy link
Member

gziolo commented Oct 31, 2019 •
edited
Loading

@gziolo Refactored the Modal story so that it doesn't open by default. It looks like that resolves the a11y issue as well 💪

It only hides them as far as I can tell :) The issue exists. I updated axe-core together with puppeteer upgrade and it reports issues with modal in many places. See:
#18205 (comment)

Travis jobs which reports it: https://travis-ci.com/WordPress/gutenberg/jobs/251398499#L1095.

FAIL packages/e2e-tests/specs/editor/various/a11y.test.js (7.621s)
  ● a11y › constrains focus to a modal when tabbing
    expect(received).toPassAxeTests(expected)
    Expected page to pass Axe accessibility tests.
    Violations found:
    Rule: "aria-hidden-focus" (ARIA hidden element must not contain focusable elements)
    Help: https://dequeuniversity.com/rules/axe/3.4/aria-hidden-focus?application=axe-puppeteer
    Affected Nodes:
      #wpwrap
        Fix ALL of the following:
          - Focusable content should be disabled or be removed from the DOM.
          - Focusable content should have tabindex='-1' or be removed from the DOM.
      178 | 	}
      179 | 
    > 180 | 	await expect( page ).toPassAxeTests( {
          | 	                     ^
      181 | 		// Temporary disabled rules to enable initial integration.
      182 | 		// See: https://github.com/WordPress/gutenberg/pull/15018.
      183 | 		disabledRules: [
      at toPassAxeTests (config/setup-test-framework.js:180:23)
          at runMicrotasks (<anonymous>)
      at Object.<anonymous> (config/setup-test-framework.js:218:2)

Anyway, it's not an issue with this PR.

It's also filed in #11631.

@gziolo gziolo mentioned this pull request Oct 31, 2019
Closed
@gziolo gziolo merged commit 085c31a into WordPress:master Oct 31, 2019
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees
No one assigned
Labels
[Feature] UI Components Impacts or related to the UI component system Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code Storybook Storybook and its stories for components
Projects
None yet
Milestone
Gutenberg 6.9
Development

Successfully merging this pull request may close these issues.
3 participants
@ItsJonQ @gziolo @youknowriad